Skip to content
This repository was archived by the owner on Nov 27, 2020. It is now read-only.

Integrate DunglasActionBundle #960

Closed
wants to merge 3 commits into from

Conversation

dunglas
Copy link
Member

@dunglas dunglas commented Mar 21, 2016

Alternative to #959.
As "actions" and __invoke seems to be confusing and a too big step fo now, let's focus on the essential for now: automatic dependency building.

action: [ ]
console: [ Command ]
directories: # List of directories relative to the kernel root directory containing classes to auto-register.
action: [ '../src/AppBundle/Controller' ] # Still an autowiring_type definition to add in Symfony to avoid this
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be great to use symfony/finder in your bundle to be able to use something like ../src/*/Controller instead.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should default to something like that, to avoid registering controllers and services from third-party bundles. I'll do it. That way, we can easily map Controller and Command without any risk to interfere with commands and controllers defined by public bundles.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's worth any extra overhead, and there's no precedent for it in other service imports.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't expect the configuration to work like this. It had never occurred to me to do any sort of autowiring across all bundles - I initially don't like that idea, and it complicates the configuration (I had to research what the difference was between the autodiscover and directories keys).

Also:

  • what causes console commands to autowire? Are the action and console keys significant? I think they are. If so, it's weird to me to also allow other keys - like foo (used in the bundle README). I understand that this is to allow other directories to be autowired, but I don't like that sometimes the keys are special, and sometimes not. It also means we can't catch typos.
  • You mentioned Still an autowiring_type definition to add in Symfony to avoid this - what
    does this refer to?
  • Would it be possible to avoid the special action-annotation route type altogether? In other words, extend @Route to use a service definition for the controller if one exists? I ask because it feels like there are two ways to auto-register a controller as a service: either by using some config in config.yml or by using the action-annotation. The line between what each does is blurry. I think extending @Route may be a non-trivial task, but it makes a lot more sense to me: it's in the spirit of autowiring where we (in this case the @Route system) effectively "asks" the container for the service for an instance and it returns it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, I am cautiously interested about all of this - my criticisms are because if it we were to go to the trouble of supporting this, let's do it really well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I see now that action-annotation does nothing more than auto-set the _controller to the action.* pattern. That feels weak.

A more generic solution where @Route just works with service controllers looks even more appealing. In fact, if we could pull it off, then it could work in YAML too using the existing syntax - e.g.

homepage:
    path: /
    defaults:
        # or using the A:B:C syntax
       _controllers: AppBundle\Controller\MainController::homepage

If that class were represented as a service, it could/would use that service automatically. Otherwise, it would create a new instance like now.

I'm assuming you've thought about this - it would require a service that contained a run-time class => service id map for resolution.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with most of your arguments and I've made changes accordingly in the bundle:

I'm not confortable with adding some magic in the router to guess the service corresponding to the type. The router supports services for a while using the 'service_name:method' syntax for while. It's what use the bundle. I prefer to keep it as is it will be easier to debug.

For the missing autowiring_type, it's for KernelInterface but it's not related anymore to this PR.

@dunglas dunglas changed the title [WIP] Integrate DunglasActionBundle Integrate DunglasActionBundle Mar 25, 2016
@@ -66,3 +66,5 @@ swiftmailer:
username: "%mailer_user%"
password: "%mailer_password%"
spool: { type: memory }


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

empty lines should be removed

@wouterj
Copy link
Member

wouterj commented Mar 25, 2016

I'm still not very sure if this is a great move for the standard edition. It can of course be great to use this bundle while developing applications, but the boilerplate code in the standard edition plays a big role in the learning process of a developer. Automatically injecting the container and having access to a get() method in the controller is already magic for beginners, imagine what happends when seeing this automatic injection stuff (as we discussed on the hackday, magic is great to advocate the framework and when you understand what's going on, but it only causes confusion in the months/years between these two phases).

Also, I'm against recommending controllers as service as the best practice. Most of the time, it only makes ones life harder (there are lots of questions about controller as services on IRC and not that much about normal controllers). Anyway, that's just my personal opinion, so feel free to just ignore it :)

@fabpot
Copy link
Member

fabpot commented Mar 26, 2016

Just for the record, I'm also very against controllers as services since they were introduced. I still think this is an anti-pattern.

@dunglas
Copy link
Member Author

dunglas commented Mar 26, 2016

@wouterj, as one of the main documenter of Symfony, it's important to convince you of benefits of this approach.

the boilerplate code in the standard edition plays a big role in the learning process of a developer. Automatically injecting the container and having access to a get() method in the controller is already magic for beginners, imagine what happends when seeing this automatic injection stuff

IMO, the "new" controller system is less magical and less Symfony-specific than the current one. And so, it is less difficult to explain it to complete newcomers:

Currently:

  • The Controller directory is a magic directory where controller classes must be put
  • Those classes usually (even if it's not mandatory) extend a Symfony specific class (the abstract controller)
  • To reference this class, you must learn an uncommon Symfony specific syntax (MyBundle:MyController:myAction with a lot of possible misunderstanding (for instance, I often forgot to remove the mandatory Action suffix of controller methods when I configure the routing)
  • To register a service, you must create a Symfony specific config file (YAML or XML service definition)
  • To access this service, you need to learn the API of the Symfony specific container

It's a lot to learn and it's mandatory even to create a small application. Many parts are not obvious when you are a newcomer. It's obvious for us because we use Symfony for years, but when I teach Symfony to students having only a small Java background, I can say that it's hard to dive into the framework.

IMO, all those stuffs to learn initially in Symfony partially explain that many developers decide to start with Laravel instead of Symfony (you dive very easily into Laravel - and their marketing is awesome but it's not related).

On the other hand, here is how the new system works:

  • The Controller directory is still a magic directory where controller classes must be put (nothing different here)
  • Those classes are just POPO, you can use a helper trait to inject common dependencies if you want to, but there is nothing magic here (at least, less than in the current solution)
  • To reference this class, you just use its FQN in the routing system, just use it's FQN (no Symfony specific syntax here). Even if we all know that is a trick (the service name is the same than class name), it's elegant and easy to learn for newcomers
  • To register and access a service, just create a class anywhere in your project, inject it in the constructor of your controller, and you're done (no XML or YAML syntax to learn, no config file to write, intuitive approach already used in many other libraries including Laravel and PHP DI, and common in other programming languages too, no dependencies to Symfony specific things like the container or the abstract controller class)

IMO (but I can be wrong), it's far less concept to learn to dive into Symfony, and it's an improved developper experience:

  1. create your logic business in POPO as usual
  2. create a class in the Controller directory
  3. type hint dependencies you need in your controller, Symfony automatically give you access to an instance of it
  4. create any method returning a Response instance and map it to a route

You have a working Symfony application.

How can it be easier?

Also, I'm against recommending controllers as service as the best practice. Most of the time, it only makes ones life harder (there are lots of questions about controller as services on IRC and not that much about normal controllers). Anyway, that's just my personal opinion, so feel free to just ignore it :)

Controller as a service is an implementation detail of my bundle, we don't have to (and we shouldn't) introduce this notion to newcomers (I haven't mentioned it in the first part of my comment).

@dunglas
Copy link
Member Author

dunglas commented Mar 26, 2016

@fabpot I usually never use controller as a service too in private projects. As explained in my previous comment the goal here isn't to promote controller as a service as best practice (it can be a "hidden" detail). It's just a way to enable automatic object construction trough autowiring.

@gnugat
Copy link

gnugat commented Mar 26, 2016

@dunglas: I know it might not sound that good, but would you consider creating a "Symfony Action Edition"? It could be promoted in the list of distributions.

@Pierstoval
Copy link
Contributor

@dunglas: I know it might not sound that good, but would you consider creating a "Symfony Action Edition"? It could be promoted in the list of distributions.

There's a dedicated discussion about editions in symfony/symfony-installer#39

@gquemener
Copy link

Just adding my personnal thought, this is the same debat as the one that occurs about the KnpLabs RAD (Rapid Application Development) Bundle a few years back.

This bundle intends to add many auto registration features (by following simple convention) among other things and question was raised about integrating it in the Symfony Standard Edition.

The point was then that Symfony Standard was not opinonated and thus would never rely on such project.

Solution was to create our own edition, which is a pretty acceptable solution IMO.

@Pierstoval
Copy link
Contributor

@gquemener thanks for pointing the fact that you created a special Symfony edition, because it is another argument in the debate about Symfony editions created in symfony/symfony-installer#39 😉

@GuilhemN
Copy link
Contributor

What do you think about instead adding a new config option to the FrameworkBundle reproducing DunglasActionBundle's config ?
It would be disabled by default, wouldn't force people to install a new bundle for such small feature and wouldn't change any convention.
It would not be focused on controllers (it looks like it was the main concern?) and people who would like to use it would have to explicitly choose the directories autowired.

I think this is the best option as it doesn't force us to modify the standard edition temporarily (as DunglasActionBundle would probably have been imported in the core after some time), neither to change conventions, change bests standards and whatever. This would only be a new feature that may be used or not.

@wouterj
Copy link
Member

wouterj commented Aug 10, 2016

Why would FrameworkBundle need Dunglas's bundle config, but not that of FOSRestBundle for instance?

@GuilhemN
Copy link
Contributor

@wouterj I don't think we can make comparisons here as FOSRestBundle is very specific to API management whereas DunglasActionBundle can be used in any project.

@wouterj
Copy link
Member

wouterj commented Aug 10, 2016

Alright, why would Frameworkbundle need Dunglas's bundle config, but not that of TacticianBundle?

In order words, Dunglas's bundle is nice, but I have no idea why it is more "core", more "nice", more "important to show to Symfonyians" than other bundles around there. Why would Dunglas's bundle need such special treatment?

@GuilhemN
Copy link
Contributor

GuilhemN commented Aug 10, 2016

I don't think we need two interfaces for the same thing (the standard CommandInterface and CommandBus). It could be in the core but the current implementation would probably have to be deprecated to avoid having to support several interfaces doing the same thing.

BTW we're not talking about the same amount of lines. Tactician is composed of many files, here we're talking about maybe two hundred lines without the tests.

But I insist on the fact that it wouldn't be a new standard but only a new feature.

Edit: ok, i got the principle of tactician.
I still don't think this is the same: Command bus is a pattern and every developer uses its own patterns.
Imo the core should only contain features that a significant part of the users may use and that won't have to change frequently (in case another pattern becomes the standard for example).

@bocharsky-bw
Copy link
Contributor

What's problem simply to add this bundle manually with $ composer require and configure it? It takes a few minutes. I don't think it's a big deal. If it is - look at KnpLabs/KnpRadBundle. It provides rapid development with Symfony and has many cool features, but I don't think to have it out-of-the-box is a good idea.

@GuilhemN
Copy link
Contributor

@bocharsky-bw I don't think an entire bundle should be created and loaded for such small feature.
I also think that features that can be used by a significant part of the community should be in the core (as long as they don't add too much complexity).
BTW when something is in the core we know it is well supported/debugged and that we can receive feedbacks when changing something.

@dunglas
Copy link
Member Author

dunglas commented Aug 10, 2016

The problem can be summarized by this tweet: https://twitter.com/lsmith/status/763074867919028224

Having a feature in the core (or in an officially supported bundle) instead of in a third-party, community maintained, repository makes a huge difference in term of marketing. Laravel has a feature like this one builtin, Laravel has a CQRS-like system builtin, Laravel has autowiring (and we now have it in core too - that makes a difference, some discussions appeared on Reddit PHP recently mentioning that Symfony now has autowiring builtin too).

Symfony is the undisputed leader in the field of PHP middlewares. It competes with J2EE and is the foundation of high-quality projects including Laravel itself, Drupal and many more. That's great.

But Symfony also has everything to be at the same time an excellent RAD framework. It only lacks some marketing, branding, official support and advertising.

Importing such feature in core will help.

@nilportugues
Copy link

This is no minor discussion but I agree on @dunglas' points.

If for instance, is a matter of rebranding this bundle, do so. Having an edition for this is a no-go as it is just causing pointless fragmentation.

@dunglas
Copy link
Member Author

dunglas commented Jan 15, 2017

See #1034

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants